Add pki_delegatee to User metadata#44873
Add pki_delegatee to User metadata#44873albertzaharovits merged 9 commits intoelastic:proxied-pkifrom
Conversation
|
Pinging @elastic/es-security |
|
I haven't reviewed the code, but I don't think May something like |
|
Thank you for the feedback Tim! I have went with one of your options, I have also added tests for it. The tests will conflict with #44561 but it doesn't matter, I'll fix the merge conflict on whatever PR gets merge the second one. |
|
@elasticmachine run elasticsearch-ci/2 |
tvernum
left a comment
There was a problem hiding this comment.
I did leave a suggestion about preventing an accidental null delegate.
I also prefer delegated_by rather than _from but that's a bikeshed, so it's up to you.
| } | ||
|
|
||
| public X509AuthenticationToken(X509Certificate[] certificates, boolean isDelegated) { | ||
| public X509AuthenticationToken(X509Certificate[] certificates, Authentication delegateeAuthentication) { |
There was a problem hiding this comment.
WDYT of making this constructor private, and adding a factory method like:
public static X509AuthenticationToken delegatedToken(X509Certificate[] certificates, Authentication delegateeAuthentication) {
Objects.requireNotNull(delegateeAuthentication);
return new X509AuthenticationToken(certificates, delegateeAuthentication);
}
I worry about constructors where passing in a null is allowed, but completely changes the behaviour. You can't guard against null (because it's allowed) but if someone accidentally passes in a null value then they will get a behaviour that doesn't do what they want (and could end up being a security issue).
There was a problem hiding this comment.
Great idea to make creating delegated tokens more explicit.
|
I went with |
This adds the
pki_delegateefield to theUser#metadatamap. The field contains the authentication of the proxy client (eg.kibana_system). The intention is for the es-admin to be able to write role mapping rules that distinguish between users authenticated directly or by delegation.Please review #44561 first, as this needs tests that require the HLRest client introduced there.
Relates #34396